-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(v2): allow specifying TOC max depth (themeConfig + frontMatter) #5578
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few thoughts on my own implementation :)
packages/docusaurus-theme-classic/src/theme/hooks/useTOCHighlight.ts
Outdated
Show resolved
Hide resolved
}: TOCHeadingsProps): JSX.Element | null { | ||
const {tableOfContents} = useThemeConfig(); | ||
const maxDepth = tableOfContents?.maxDepth ?? 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a cleaner way of handling default config values? I couldn't find where exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you've already added a default value in Joi validation, there's no need to duplicate it here (Joi validation actually mutates the object)
Would it make sense to add a minDepth
option as well? Would the user sometimes not want to have h2
headings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you've already added a default value in Joi validation, there's no need to duplicate it here (Joi validation actually mutates the object)
Weird because I get the following error when I don't have the null coalescing part:
TypeError: Cannot destructure property 'maxDepth' of 'tableOfContents' as it is undefined.
git diff
of the code change:
diff --git a/packages/docusaurus-theme-classic/src/theme/TOC/index.tsx b/packages/docusaurus-theme-classic/src/theme/TOC/index.tsx
index 83c3d2930..4dfe9038c 100644
--- a/packages/docusaurus-theme-classic/src/theme/TOC/index.tsx
+++ b/packages/docusaurus-theme-classic/src/theme/TOC/index.tsx
@@ -28,7 +28,7 @@ export function TOCHeadings({
depth: headingLevel,
}: TOCHeadingsProps): JSX.Element | null {
const {tableOfContents} = useThemeConfig();
- const maxDepth = tableOfContents?.maxDepth ?? 3;
+ const { maxDepth } = tableOfContents;
if (!toc.length) {
return null;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to add a minDepth option as well? Would the user sometimes not want to have h2 headings?
Personally I think this feels like an anti-pattern since you generally want to utilize all markdown heading levels. I could see it for some niche cases like having a single h2
on a page and not wanting to nest all of your h3
inside, but feels funny to have it on site-wide in the theme config.
Happy to implement it if others feel differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird because I get the following error when I don't have the null coalescing part
Oh yeah, you need to provide a default value for the entire tableOfContents
config object as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but feels funny to have it on site-wide in the theme config.
I'm thinking about adding a per-doc option minLevel
as well, and just for the sake of uniformity, I think a site-level config is worth it. But seems @slorber doesn't particularly like too many APIs, so let's see🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you've already added a default value in Joi validation, there's no need to duplicate it here (Joi validation actually mutates the object)
Addressed this in 0785423
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency I'd rather have min/max everywhere, even if min is less likely to be used and mostly for weird use-cases
✔️ [V2] 🔨 Explore the source changes: 3db8779 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61541f09a7aef900077789b1 😎 Browse the preview: https://deploy-preview-5578--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5578--docusaurus-2.netlify.app/ |
let parentIndex = prevParentIndex; | ||
// We start at the parent of the previous heading | ||
// Recurse through its ancestors until the current heading would be a child | ||
while (parentIndex >= 0 && currLevel < headings[parentIndex].level) { | ||
parentIndex = headings[parentIndex].parentIndex; | ||
} | ||
return parentIndex >= 0 ? headings[parentIndex].parentIndex : parentIndex; | ||
}; | ||
|
||
visit(node, 'heading', visitor); | ||
// Assign the correct `parentIndex` for each heading. | ||
headings.forEach((curr, currIndex) => { | ||
if (currIndex > 0) { | ||
const prevIndex = currIndex - 1; | ||
const prev = headings[prevIndex]; | ||
if (curr.level > prev.level) { | ||
curr.parentIndex = prevIndex; | ||
} else if (curr.level < prev.level) { | ||
curr.parentIndex = getParentIndex(prev.parentIndex, curr.level); | ||
} else { | ||
curr.parentIndex = prev.parentIndex; | ||
} | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a better solution here: record "the nearest h2
~h6
node indices" as an array closestNode[]
where closestNode[level]
is the index of the closest node with level h<level>
and closestNode[0] = closestNode[1] = -1
.
On each node you find the value in closestNode
with a smaller level than curr.level
and the largest index possible, and then update closestNode
:
const closestNode = Array(7).fill(-1);
headings.forEach((curr, currIndex) => {
curr.parentIndex = Math.max(...closestNode.slice(0, curr.level));
closestNode[curr.level] = currIndex;
});
Of course this would mean a little inline comments, but the code is much shorter and also more straightforward IMO
Invited from Discord :P LGTM, left some inline comments. Just realized that I gave two contradictory suggestions: using I'd also want a |
@erickzhao thank you for tackling this issue! Yeah, overriding/setting TOC level on per Markdown doc basis would be awesome. Are you interested in doing this? |
I'll take a look at implementing the per-doc frontmatter config over the weekend. Thanks for the tips! :) |
I just realized that this current recursion-based approach is flawed if the Markdown doesn't follow W3C heading level guidelines:
e.g. If someone skips from I see 3 possible solutions to handle incorrectly formatted heading levels: 1️⃣ Refactor the I think 1️⃣ is the best solution if we do want to also expose the per-document |
Yes, 1️⃣ would probably work the best, but I think 3️⃣ will also make sense |
headings.forEach((curr, currIndex) => { | ||
// take the last seen index for each ancestor level. the highest | ||
// index will be the direct ancestor of the current heading. | ||
const ancestorLevelIndexes = prevIndexForLevel.slice(2, curr.level); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Math.max(...prevIndexForLevel.slice(2, 2)) = Math.max(...[]) = -Infinity
, although that doesn't break the algorithm, does it really make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes as much sense to me as having indexes for "levels" 0 and 1, I figure. It's nice to be explicit about only starting at index 2 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤪 my head hurts 🤯 will have to take more time reviewing this.
We need more test cases to cover the edge cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested a few edge cases locally (never committed the changes) and it seemed to work somewhat well.
My personal take (as a drive-by contributor, feel free to ignore) is that while the TOC should never break, documents that have wonky heading formatting won't suffer too much from having a wonky TOC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, just added tests to ensure the behavior with wonky TOC stays the same over time :)
…sts will be simpler to write
This feature is ready to merge! Let me know if you see any issue or I'll merge it tomorrow |
Typechecking the website coming to use😎 |
Yes, not the first error it catches, great idea 👍 |
}) { | ||
const selectors = []; | ||
for (let i = minHeadingLevel; i <= maxHeadingLevel; i += 1) { | ||
selectors.push(`.anchor.anchor__h${i}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed it earlier, can we remove this redundant anchor__X
class for each of headings levels? Maybe use more specific name like docusaurus_anchor
instead of just anchor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure to understand what you mean here.
We want to get only the anchors from min to max, so we need levels in classnames otherwise we'd select more anchors and it could mess-up with the TOC algorithm (an anchor that is not in the TOC should never be considered as "active" because we wouldn't be able to highlight it because it doesn't appear in the TOC)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could mess-up with the TOC algorithm
How can something break if we get all anchor links? If it important, we can actually filter out anchor links based on its heading tag instead class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is important. Consider this scenario:
- We config to only use h2 in toc (not h3)
- We scroll to a h3 heading
Then, the h3 heading is considered the "active" one but it's not in the TOC, so nothing is highlighted (while we should highlight the parent h2)
we can actually filter out anchor links based on its heading tag instead class.
You can do select("h2 h3 h4")
or select("*").filter(is("h2 h3 h4"))
, both would lead to the same result
It's a best practice to create a more precise selector / query in the first place when possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both would lead to the same result
Yes, but we can get rid of extra classes, why do we need them if can avoid using them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem like a big deal to me to keep them and would be a CSS breaking change to remove them now
Also nothing guarantees that a user won't use headings in other places of its theme for weird reasons, and we may want to avoid that it conflicts with the TOC algorithm by not using "h2 h3"
selector on purpose. Classnames help remove the opportunities for such a conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've already marked this PR as BC, so it's good time to get rid of this extra thing.
Right, but our header-anchors will still have generic class anchor
, though could make it even more specific. I suggest removing only heading-specific classes, and use heading tag level instead in TOC algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I thought you wanted to remove both classes
If you want to refactor to h2.anchor, h3.anchor
, I'm fine with the idea (and .anchor__h2
is quite new so it's a smaller breaking change)
Feature started by @erickzhao, completed by @slorber
Breaking changes
@theme/TOCItems
component@theme/BlogLayout
:toc
prop now accepts a<TOC>
ReactNode instead of the toc json object.useTOCHighlight
to theme-common packageMotivation
Fixes #2700
This PR is based on the implementation suggestion from #4310 (review):
New frontmatter toc heading level config:
New themeConfig toc heading level (for default levels)
Example of
toc_max_heading_level:5
on https://docusaurus.io/docs/i18n/crowdinHave you read the Contributing Guidelines on pull requests?
Yep.
Test Plan
Unit tests
Dogfooding, deploy preview links:
Related PRs
See #4310 for original implementation